Skip to content

Conversation

@otekraden
Copy link

@otekraden otekraden commented Nov 14, 2025

I solved Issue #2559 by creation the function "format_message_to_asterisk_box", that formatting message to asterisk box representation.

It takes this args:
message (str): The message to format.
title (str, optional): The title of the box. Defaults to None.
width (int, optional): The width of the box. Defaults to 80.
border_char (str, optional): The character to use for the border.
Defaults to "*".
padding (int, optional): The number of spaces to pad the border.
Defaults to 2.

Also I added tests for it. Function it seems working good.

But I was not sure in what file to put function.
Now its location and tests by this paths:
src/briefcase/utils.py
tests/test_utils.py

It is my very first attempt of contribution. Please don beat me hard)

I am waiting for your feedback.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@otekraden otekraden marked this pull request as draft November 15, 2025 00:56
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! The broad strokes of this look like it's on the right track - but there's a few details to sort out. Some of them I've flagged inline; but there are a couple of bigger picture items.

Firstly, this is currently failing CI because of a town crier check; see our contribution guide for details on what that means.

Secondly - we try to avoid generic "utils" modules. In this case, the code is a very specific addition to console handling - so it should be part of the console module. Given that this will always be used in the context of a console.warning(), it would make sense to me for this to be a standalone method on Console - something like console.warning_banner(title, body, width=80). It may help to keep the "formatting" part of the code separate so that console._banner() returns a string, which is then output by console.warning_banner() (and provides an opportunity for error_banner() if we ever need it).

Defaults to 2.
Returns:
str: The formatted message.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the Sphinx docstring format; check any other source file for examples of use.

Copy link
Author

@otekraden otekraden Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!
I applied this format.

lines.append(border_line)

# split the message into paragraphs
paragraphs = message.split("\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use textwrap.dedent() here as a convenience as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but in my current understanding of the logic of my function, I don't see how to use the textwrap.dedent() method. Could you please show me how to integrate it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm suggesting is that being able to call:

        console.warning_banner(
            "Some banner title"
            """\
                This is the content that I want to display. It is long
                but I don't want to have to manually word wrap\n
                \n
                But manual line breaks should be preserved.
            """
        )

We could do this by requiring the user to wrap the """ string in textwrap.dedent()... or we could do that automatically on every usage.

@otekraden
Copy link
Author

@freakboy3742
Thank you for your feedback.
As expected, it turned out to be not so easy.
I've seen that the CI process crashes into an error, but I haven't figured out how to fix it yet.
I will take all your comments into account and finalize this pull request in the near future.

Otekraden added 2 commits December 2, 2025 22:52
…sole primitives section. Tests relocated to Console tests folder.
@otekraden otekraden marked this pull request as ready for review December 2, 2025 20:07
@otekraden otekraden marked this pull request as draft December 2, 2025 20:08
@otekraden
Copy link
Author

otekraden commented Dec 2, 2025

I have attempted to make all the necessary changes based on the instructions above.
The function has been transformed into a static method "warning_banner" in the Console primitives section.
Tests have been relocated to the Console tests folder.
Currently, CI towncrier works fine, but there seems to be an issue deeper in the app building process.

@otekraden otekraden marked this pull request as ready for review December 2, 2025 20:56
@otekraden
Copy link
Author

I rewrote Console method from static to instance method.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those cleanups. The CI failure you saw was unrelated - we had some disk space issues in our CI configuration. We've fixed those now, and I've re-run the affected tests, and they're passing now.

I've left a couple more review comments; the only other suggestion is that it could use a couple more tests.

In particular, this is a situation where parameterized tests would be helpful. There's a general pattern of a set of input. Then a need to test how that input renders at different widths - widths that are longer than the text, the same width as the text, less than the width of the text, and so on.

Lastly, it would be useful to actually use this API somewhere. It doesn't need to update every usage of banner-like output, but if you pick a module (like the Android integrations), and replace every warning banner with the equivalent usage of this utility, we can see that it will work in practice.

Comment on lines 263 to 275
:param message: The message to format inside the box.
:type message: str
:param title: The title of the box. If provided, appears centered at the top.
:type title: str, optional
:param width: The total width of the box in characters. Defaults to 80.
:type width: int, optional
:param border_char: Character to use for the box border. Defaults to "*".
:type border_char: str, optional
:param padding: Number of spaces between the border and text content. Defaults
to 2.
:type padding: int, optional
:return: The formatted message enclosed in a bordered box.
:rtype: str
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the miscommunication here; but we don't need the type documentation - because they are (or should be) in the type annotations for the method itself.

Suggested change
:param message: The message to format inside the box.
:type message: str
:param title: The title of the box. If provided, appears centered at the top.
:type title: str, optional
:param width: The total width of the box in characters. Defaults to 80.
:type width: int, optional
:param border_char: Character to use for the box border. Defaults to "*".
:type border_char: str, optional
:param padding: Number of spaces between the border and text content. Defaults
to 2.
:type padding: int, optional
:return: The formatted message enclosed in a bordered box.
:rtype: str
:param message: The message to format inside the box.
:param title: The title of the box. If provided, appears centered at the top.
:param width: The total width of the box in characters. Defaults to 80.
:param border_char: Character to use for the box border. Defaults to "*".
:param padding: Number of spaces between the border and text content. Defaults
to 2.
:return: The formatted message enclosed in a bordered box.

Comment on lines 257 to 259
width=80,
border_char="*",
padding=2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing type annotations:

Suggested change
width=80,
border_char="*",
padding=2,
width: int = 80,
border_char: str = "*",
padding: int = 2,

# Wrap the title to lines to fit the width of the box
wrapped_title_lines = textwrap.wrap(
title.strip(),
width=width - (padding * 2) - 4,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure there's a use case for configurable padding here - 1 space of padding in the title will always be the default.

lines.append(border_line)

# split the message into paragraphs
paragraphs = message.split("\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm suggesting is that being able to call:

        console.warning_banner(
            "Some banner title"
            """\
                This is the content that I want to display. It is long
                but I don't want to have to manually word wrap\n
                \n
                But manual line breaks should be preserved.
            """
        )

We could do this by requiring the user to wrap the """ string in textwrap.dedent()... or we could do that automatically on every usage.

@otekraden
Copy link
Author

otekraden commented Dec 23, 2025

I have made the following changes:

  • added the ability to enter a message and title as a multiline or single line. At the same time, it is possible to manually split texts into paragraphs using "\\n";
  • did parametric tests;
  • corrected docstring and arg types.

@kattni
Copy link
Contributor

kattni commented Dec 27, 2025

@otekraden Hello! Thank you for submitting the requested changes. As you can see, the CI is failing. We maintain 100% code coverage in testing. The current CI issue appears to be related to the PR now being less-than 100% coverage. You can click on the failure to see exactly where the issue is. Please get this PR passing before we review it again. If you have questions or need help, please post here. Thanks!

@otekraden
Copy link
Author

@kattni @freakboy3742
Hello! I need your help with tests coverage.
I tried everything but string 324 in console.py not covered((

image

I will be grateful for any hint.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good! A few review comments inline.

Regarding the coverage failure - the error is telling you that a specific line of code isn't being exercised by the test suite. The fix is to add a test case that will exercise that line of code - in this case, the case of a message that isn't a string.

That said - as noted by my review comments, that line of code probably isn't needed, because we can assume the method is being used by a "reasonable person" who is working with Briefcase. It doesn't necessarily need to be robust enough to handle any random input from an end user.

The last suggestion - at present, this PR adds a new feature, and does a good job testing; however, it would be good to see a few examples of the API being used in practice. You don't need to update every banner in the codebase - but if you pick one module (say, the Android SDK integrations in src/briefcase/integrations/android_sdk.py) and replace all banners with usage of this API, you can prove that the API is "fit for purpose".



@pytest.mark.parametrize(
("test_name", "message", "title", "width", "expected"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to include a descriptive test name - your test doesn't use it anyway:

Suggested change
("test_name", "message", "title", "width", "expected"),
("message", "title", "width", "expected"),

If you do want to provide a test identifier, use pytest.param() to define each group of test data; the ID in that case should be something short (like "test-1" or "empty-message-title") that can be used to run a specific test. This isn't required, but it can be a nice convenience.

experience in open source
development.
I like your project, and I'm excited to contribute to it.\
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the use of an f-string testing anything specific here? I'm fairly sure this can be simplified to a static string definition.

80,
"""\
********************************************************************************
** **
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra blank link in the title is a little odd. It wouldn't match any existing uses of warning banners.

Comment on lines +205 to +211
# # Debug output if needed
# print('\n\n' + test_name)
# print("Generated:")
# print(msg)
# print("Expected:")
# print(expected)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to retain this debug code.

Suggested change
# # Debug output if needed
# print('\n\n' + test_name)
# print("Generated:")
# print(msg)
# print("Expected:")
# print(expected)

if not any((message, title)) or not isinstance(width, int):
return ""

def dedent_and_split(text):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than defining this as an inline function, it should be a private method on the base Console class (i.e. , _dedent_and_split().

I'd also suggest that we can add an extra affordance around the handling of input strings. As written, I think this requires that the input string has a trailing \ on the first line (i.e., warning_banner(f"""\. That's the sort of detail that will be consistently forgotten in practice. Since the effect of the \ is to suppress the newline on the first character, it might be worth adding a text.strip('\n') so that any leading or trailing newlines are removed from the input string, ensuring that we're in complete control of the spacing around the message in the box.

Comment on lines +274 to +275
if not any((message, title)) or not isinstance(width, int):
return ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to raise an error if the arguments aren't the right type?

It's also worth asking if any error handling needed at all. This isn't a public API - it's an internal utility; we don't have to be rock solid in the API we expose, because we're in control of how the method is used.

Comment on lines +293 to +294
if not isinstance(title, str):
return ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're doing error handling, we should be raising an error, not failing silently.

But again - is any error handling needed?

Comment on lines +323 to +324
if not isinstance(message, str):
return ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above - is this error handling needed; and if it is, shouldn't it be error handling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants